library-manager: use Kconfig for INTEL_MODULES #9936
Conversation
lyakh
requested review from
abonislawski,
dbaluta,
iuliana-prodan,
kv2019i,
lbetlej,
lgirdwood,
mmaka1 and
plbossart
as code owners
softwarecki
left a comment
softwarecki
left a comment
There was a problem hiding this comment.
The changes in the makefile seem unrelated and just organizational. Maybe it's worth moving them to a separate commit and describing them somehow?
| drv->ops.dai_ts_stop = module_adapter_ts_stop_op; | ||
| drv->ops.dai_ts_get = module_adapter_ts_get_op; | ||
| #if CONFIG_INTEL_MODULES | ||
| drv->adapter_ops = &processing_module_adapter_interface; |
There was a problem hiding this comment.
You can't just not assign a adapter_ops pointer because it will end in a firmware crash when trying to load the iadk library.
There was a problem hiding this comment.
@softwarecki IADK libraries cannot be loaded with firmware built with CONFIG_INTEL_MODULES=n, so no crash expected
There was a problem hiding this comment.
@lyakh Attempting to load a iadk module in firmware built with CONFIG_INTEL_MODULES=n results in a crash in function lib_manager_module_create on call module_adapter_new.
[ 0.000173] <err> os: print_fatal_exception: ** CPU 0 EXCCAUSE 13 (load/store PIF data error)
[ 0.000173] <err> os: print_fatal_exception: ** PC 0xa00632c7 VADDR 0xa00303c4
[ 0.000173] <err> os: print_fatal_exception: ** PS 0x60d20
[ 0.000173] <err> os: print_fatal_exception: ** (INTLEVEL:0 EXCM: 0 UM:1 RING:0 WOE:1 OWB:13 CALLINC:2)
[ 0.000173] <err> os: xtensa_dump_stack: ** A0 0xa104a5b0 SP 0xa00a4660 A2 0xa00f51c4 A3 0xa00a46a4
[ 0.000173] <err> os: xtensa_dump_stack: ** A4 (nil) A5 0xa00f51c0 A6 0x400f5100 A7 0xa00a4660
[ 0.000173] <err> os: xtensa_dump_stack: ** A8 0xa00631ec A9 (nil) A10 (nil) A11 0xa00f51c4
[ 0.000173] <err> os: xtensa_dump_stack: ** A12 0x60 A13 0x400f53a0 A14 0xa0026098 A15 0x1
[ 0.000173] <err> os: xtensa_dump_stack: ** LBEG 0xa006daaa LEND 0xa006dae0 LCOUNT (nil)
[ 0.000173] <err> os: xtensa_dump_stack: ** SAR 0x12
[ 0.000173] <err> os: xtensa_dump_stack: ** THREADPTR 0x1
There was a problem hiding this comment.
@softwarecki aha, this is interesting! We'd expect an error response instead of a crash. Let's try to fix this, thanks!
There was a problem hiding this comment.
@lyakh Attempting to load a iadk module in firmware built with
CONFIG_INTEL_MODULES=nresults in a crash in functionlib_manager_module_createon callmodule_adapter_new.
@softwarecki should be fixed in the updated version
marcinszkudlinski
left a comment
marcinszkudlinski
left a comment
There was a problem hiding this comment.
@lyakh "intel modules" need to be operational, even when rarely used. There's even a test for this in internal CI - clearly not working properly as CI has passed - anyway we cannot just turn it off
@softwarecki they are related - without them it's impossible to disable IADK while keeping LLEXT enabled. But yes, it could be split into 2 - first make it possible to enable individually, then disable where unneeded |
|
@lyakh CI tests have been fixed and re-run, we can now see that the tests loading IADK are failing as we expected |
lyakh
changed the title
lyakh
changed the title
|
@marcinszkudlinski @softwarecki please check, not disabling it now, but fixing Kconfig to make it possible |
lgirdwood
changed the title
| endif() | ||
|
|
||
| zephyr_include_directories_ifdef(CONFIG_INTEL_MODULES | ||
| zephyr_include_directories_ifdef(CONFIG_LIBRARY_MANAGER |
There was a problem hiding this comment.
Would this not be something like the original CONFIG ? Dont we want to preserve the original Kconfig to select IADK API ?
There was a problem hiding this comment.
@lgirdwood not sure I understand correctly. Currently my understanding is, that CONFIG_LIBRARY_MANAGER is needed always when using the library manager, i.e. for both LLEXT and IADK. While CONFIG_INTEL_MODULES is only needed for IADK. So, here I change it to the former to make LLEXT usable without the latter enabled.
There was a problem hiding this comment.
So we need sof/audio/module_adapter/iadk/ directory just with CONFIG_LIBRARY_MANAGER=y?
looks like it should be included only with CONFIG_INTEL_MODULES=y
There was a problem hiding this comment.
@abonislawski unfortunately yes. Obviously, I tried building SOF with LLEXT modules with CONFIG_INTEL_MODULES=n and then I got errors about headers not being found. When I removed IADK headers, I got undefined types for some structures. We can work more on this to decouple non-IADK use-cases from IADK headers, but in this first step I didn't go all the way to do that, it would involve for me making less trivial changes to code that I cannot test locally
lyakh
force-pushed
the
iadk
branch
2 times, most recently
from
fd722a0 to
d0f40d9
Compare
IADK is rarely needed, but when enabled it adds C++ objects to the build, while otherwise builds are pure C. Disabling INTEL_MODULES however is currently broken. Fix it to make modular builds without IADK support possible. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
softwarecki
dismissed
their stale review
I think we now correctly handle attempting to load the iadk module when support for them is disabled.
marcinszkudlinski
left a comment
marcinszkudlinski
left a comment
There was a problem hiding this comment.
Now the PR is fixing handling of CONFIG_INTEL_MODULES=n
Ok!
|
CI:
|
6 participants
IADK is rarely needed, but when enabled it adds C++ objects to the build, while otherwise build are pure C. Disable INTEL_MODULES by default on ACE platforms.